-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Toggle button for file previews #3290
Conversation
fiskus
commented
Jan 24, 2023
•
edited
Loading
edited
- Changelog entry (skip if change is not significant to end users, e.g. docs only)
Codecov Report
@@ Coverage Diff @@
## master #3290 +/- ##
==========================================
- Coverage 35.30% 35.26% -0.04%
==========================================
Files 666 668 +2
Lines 29234 29267 +33
Branches 4336 4340 +4
==========================================
Hits 10322 10322
- Misses 17726 17759 +33
Partials 1186 1186
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine, couple thoughts:
- search results display is not affected by this change, is it?
- i'd put the expand controls to the right, and hidden all the other stuff (e.g. download) into a "more" menu next to it -- this would imo work better for search results, bc search results show an entity type icon on the left (package vs object)
Does it look ok for you? I'm not sure about my opinion yet |
I put the expand/collapse button to the right, and shrank the download button into a 3-dots menu. Also, I removed "auto-expand" functionality because it requires more work: some previews obtain height after a while (Jupyter, JsonDisplay). So, we need to listen resize etc., I don't want to pollute this PR with this. |
that looked ok to me, tho the spacing btw the icon buttons might be a little tighter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code seems fine, tho im not sold on the design -- full-fledged button seems too bulky for the place imo. but it's your call if you think it's better this way
I agree, that design looks nicer without EXPAND text and bordered button, but I'm afraid user couldn't find that button. Let's let users get used to the new button position, then we can optimize the space and shrink this button. |